Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add monthly report support #6508

Merged
merged 5 commits into from
Feb 4, 2025
Merged

feat: Add monthly report support #6508

merged 5 commits into from
Feb 4, 2025

Conversation

AdityaHegde
Copy link
Collaborator

closes #6503

@AdityaHegde AdityaHegde marked this pull request as ready for review January 27, 2025 11:39
Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UXQA:

  1. The "Create" button should be disabled until the all the form's required fields are input & when there's input validation errors.
  2. The "Invalid" text should be left-aligned with the main body of the form (and it should be a more informative message).
    image
  3. The "repeats" section should be written in more natural English, e.g. "on the 31st of each month"
    image
  4. I see that when the next month doesn't have the provided day, like February 30th, the report is skipped for that month. @mindspank, can you please do the Product review for this PR? Is this what you'd expect?
    image

@ericpgreen2 ericpgreen2 requested a review from mindspank January 31, 2025 13:07
@mindspank
Copy link
Contributor

mindspank commented Jan 31, 2025

I see that when the next month doesn't have the provided day, like February 30th, the report is skipped for that month.

I would expect the report to run on the 28th in this case. Same for reports scheduled to run on 31st I would expect to run on the 30th if there is no 31 for the next month.

Alternatively if we had "First day" and "Last day" as options we can keep the existing behavior. (Realizing First day is redundant but to match the Last day option)

@AdityaHegde
Copy link
Collaborator Author

Disabled the input for now.

Comment on lines 31 to 53
const suffixMap = {
1: "st",
2: "nd",
3: "rd",
};
export function formatRefreshSchedule(reportSpec: V1ReportSpec) {
if (!reportSpec.refreshSchedule?.cron) return "";
let formattedRefreshSchedule = cronstrue.toString(
reportSpec.refreshSchedule.cron,
{
verbose: true,
},
);
formattedRefreshSchedule = formattedRefreshSchedule.replace(
/on day (\d*) of the month/,
(_, day: number) => {
const suffix = suffixMap[day % 10] ?? "th";
return `on the ${day}${suffix} of each month`;
},
);

return formattedRefreshSchedule;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a ChatGPT convo with a cleanup proposal: link

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convertFormValuesToCronExpression() and getFrequencyFromCronExpression() could use some unit tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. None of this code has any tests.

@AdityaHegde AdityaHegde merged commit 310feff into main Feb 4, 2025
7 checks passed
@AdityaHegde AdityaHegde deleted the feat/monthly-reports branch February 4, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scheduled Reports: Add option to issue report monthly
3 participants